What is the use case? When would someone want to flash an outdated image? Or is this because the no-op build is too slow? It shouldn't be...

EDIT:
cc:

• scripts: west_commands: add build.symlinks and extend build.dir-fmt #96231, @thorsten-klein

What is the use case? When would someone want to flash an outdated image? Or is this because the no-op build is too slow? It shouldn't...

The skip rebuild option already exists. I am just adding a west config option for it so that someone doesn't have to type it every time if they generally don't want it to behave that way. So I don't think it's in the scope of the PR to debate the utility of that already-existing option.

I can say though that my use case is that I often am having builds on multiple different SHAs due to I am trying to compare behaviors or something at different points in history, and using different build directory to store the builds so that they don't overwrite each other on the default path. And checking out to a different SHA causes the image to get rebuilt if it's from a different SHA, so this is really annoying because I have to keep doing git checkout just to flash/debug the proper image or else be typing --skip-rebuild all the time. So I just want a config option to make this the default if desired, which I do.

I just noticed now... I had not yet because you're adding/duplicating the config logic in a different place than the command-line flag logic. Please keep all that logic in the same place.

I'm not sure where to put it then, I just did a similar thing to how config.get is used elsewhere in this file such as for build.guess-dir

You're also adding documentation for the config far away from --skip-rebuild help, is that necessary and why?

I am following the same style as the west build documentation section right above it to be consistent about where/how we document the custom west commands' config options, you can view on https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#configuration-options

Also, the reason I even found this doc page to put it on at all, is because this page: https://docs.zephyrproject.org/latest/develop/west/config.html#built-in-configuration-options says that the doc of the west config options for the custom commands is on their specific page, so I looked up west flash on the doc site search, and this page seemed like the best result, since it had documented the west build configs also

While less so, it can still be valid to revisit the value of a feature when changing it. Not every feature was a good idea and extending bad ones is not either.

A big difference between --skip-rebuild and this new flash.skip-rebuild config is that the first one is:

• much harder to forget and miss in interactive use
• visible and persisted in build logs

Forgetting that flash.skip-rebuild is ON can waste a LOT of time. This risk must be balanced against saving a bit of typing; I'm not sure it's worth it. Surely, when testing different --build-dir you recall the previous command and do not re-enter --skip-rebuild every time, right?

Finally, why not just this instead?

--- a/scripts/west_commands/run_common.py
+++ b/scripts/west_commands/run_common.py
@@ -142,7 +142,7 @@ def add_parser_common(command, parser_adder=None, parser=None):
                        help=argparse.SUPPRESS)
     group.add_argument('-r', '--runner',
                        help='override default runner from --build-dir')
-    group.add_argument('--skip-rebuild', action='store_true',
+    group.add_argument('-s', '--skip-rebuild', action='store_true',
                        help='do not refresh cmake dependencies first')
     group.add_argument('--domain', action='append',
                        help='execute runner only for given domain')

It's not like this particular flash sub-command is anywhere close to running out of the 26 letters of the ASCII alphabet.

Please keep all that logic in the same place.

I'm not sure where to put it then

?

    if not user_args.skip_rebuild:
        rebuild(command, build_dir, user_args)

    # Re-build unless asked not to, to make sure the output is up to date.
    if build_dir and not args.skip_rebuild:
        rebuild(command, build_dir, args)

such as for build.guess-dir

I don't think build.guess-dir matters much here.

I am following the same style as the west build documentation section right

Fair enough... I don't have a strong opinion there but if not in the same place for some reason, then the documentations of --skip-rebuild and flash.skip-rebuild should definitely not look like they don't even know each other.

Forgetting that flash.skip-rebuild is ON can waste a LOT of time.

but doesn't that go for any west config command ?
If I've changed my west config manifest.path foo and forgot about it, then that also can take time to remember.

Or if setting some fixed CMake args using: west config build.cmake-args.

A west config option will not be present in the config files unless the user as explicitly set it, which makes it the users responsibility. It's simple, don't use it if you don't like it.

As for the --skip-rebuild option, then I do see the use-case presented although I wouldn't use it on west flash, but I certainly use it most of the times with west debugserver.

So granularity should be improved, so that if being able to use skip-rebuild for flash, then it should only apply to west flash, not west debug / debugserver / etc, those should support their own: west config debug.skip-rebuild / debugserver.skip-rebuild / etc as mentioned: #96115 (comment)

$ west config alias.flash -- "flash --skip-rebuild"

Nice! @decsny can you please try this? Or something like this if you prefer not shadowing:

west config alias.flashSB -- "flash --skip-rebuild"

but doesn't that go for any west config command ?

It depends: they don't all have the same cost when forgetting them; they are not all "footguns". Forgetting that you configured grep.color is never going to cost you a lot of time.

If I've changed my west config manifest.path foo and forgot about it, then that also can take time to remember.

Right, this one looks like a pretty big footgun. But unlike this PR, there is no command-line or other alternative for config manifest.path, is there? I mean it's the only way to achieve this without a git diff?

Or if setting some fixed CMake args using: west config build.cmake-args.

Agreed, this is another footgun but the effect of this one is probably easier to spot in the build logs?

Anyway, we can debate and compare footguns until the cows come home but already having an arsenal is not a good enough reason to add yet another, new footgun.

A west config option will not be present in the config files unless the user as explicitly set it, which makes it the users responsibility. It's simple, don't use it if you don't like it.

Footguns are never "simple", they is always a benefit/risk trade-off. For this particular one, the benefit seems extremely low. In every Zephyr survey I keep seeing this coming back: "make Zephyr more newbie-friendly".

Also, @pdgendt already added the "ultimate swiss army footgun" a.k.a. config.alias. So, do we really need to keep adding new ones?

PS: my "request for changes" is about the current implementation.

This could be related to my use case that users want to keep multiple different builds and switch between them:
Ref: #96231

In case of OP's use case it could make sense to extend build.dir-fmt to support {branch}, {commit} or {timestamp} to ensure a unique build folder per branch or commit or always (timestamp)

Or if setting some fixed CMake args using: west config build.cmake-args.

Agreed, this is another footgun but the effect of this one is probably easier to spot in the build logs?

I guess that depends on the user.
You can't imagine the things that people don't notice in the CMake output, even though we try to only print the most important 😞

Regarding the rebuild / no rebuild, then I would say it's just as simple to spot:

$ west flash
-- west flash: rebuilding    <--- it tries to rebuild
ninja: no work to do.
-- west flash: using runner nrfutil
...

vs:

$ west flash --skip-rebuild
-- west flash: using runner nrfutil   <---- Goes straight to flash
....

Though if approving the config setting, flash.skip-rebuild, then it probably should be highlighted, for example like this:

$ west flash --skip-rebuild
-- west flash: skip rebuild   <-- user is informed
-- west flash: using runner nrfutil
....

But there is one thing we should have if supporting west config flash.skip-rebuild true, and that is to overrule that setting on command line.

For example I can enable sysbuild by default, but deselect it on a specific build, like this:

$ west config build.sysbuild true                # Use sysbuild in general
$ west build -b plank --no-sysbuild <sample>     # Do not use sysbuild in this case

So if we're going to support west config flash.skip-rebuild true then it must be possible to overwrite this on command line, for example by using:

$ west flash --force-rebuild

Side-note: if introducing --force-rebuild, then it probably better to name the flags --build / --no-build or --rebuild / --no-rebuild.

For this particular one, the benefit seems extremely low.

Like anything else, I would say it depends.
One thing that have hit me is when trying to run samples in debugger where you might have a reference sample in build_working.
The if you do west flash -d build_working and forget --skip-rebuild, then the build from the working SHA is immediately cleared, and you need to checkout that SHA and rebuild, just to jump back to where you were.
But again, that's just an annoyance, nothing where you spend time tracing down why it's not rebuilding.

You also need to make sure the command-line takes precedence over the config.

So if we're going to support west config flash.skip-rebuild true then it must be possible to overwrite this on command line when invoking west flash. For example like this (I want my rebuild):
$ west flash --force-rebuild

I think west flash --skip-rebuild=false would be preferable. One fewer flag and "Command line has precedence over config" is more obvious than "force has precedence over skip".

I think west flash --skip-rebuild=false would be preferable. One fewer flag and "Command line has precedence over config" is more obvious than "force has precedence over skip".

I disagree, we should use --no-rebuild and --rebuild similar to many other tools/compilers using this pattern. We already have --no-sysbuild/--sysbuild too.

We even have an extendable argparse.Action class we can re-use:

That's even more confusing that double negations because it looks like that "skip" and "force" could refer to a different type of "build".

Sorry, I never intended to propose force as name in argument. Only to highlight the intention / reason for having the ability to overrule whatever setting existed in config file.
As I wrote:
Side-note: if introducing --force-rebuild, then it probably better to name the flags --build / --no-build or --rebuild / --no-rebuild.

and keep --skip-rebuild as an alias for backwards-compatibility?

I think that's a fine thing, and if we want to cleanup later then we can start deprecating --skip-rebuild.

If I understand the convergence of the discussion was to introduce --rebuild and --no-rebuild, and to make the west config xxx.rebuild generic to any command ?

Consensus makes sense to me. For the record,

. @mbolivar , any memories of --skip-rebuild?

@marc-hb consider the use case "As a developer, I have to flash multiple instances of the same board with the same binary, and I want to ensure that there are no differences in the different binaries flashed. I have an old build directory I would like to use for this purpose. My working tree has diverged significantly from when I built it."

@marc-hb although to be honest, the truth is that @SebastianBoe wanted us to rebuild by default, and he was the build system maintainer back then so I thought it best not to argue even though I disagreed. My personal preference would have been for the --skip-rebuild behavior to be the default.

Please keep all that logic in the same place.

It's not just a code style issue BTW. You also need to make sure the command-line takes precedence over the config.

I'm actually not sure what you're referring here though because one of the things I did on the original commit here is refactor to try to put all the logic in the same place, since there was some duplication across the file, maybe you can be more specific of what you mean?

It's not just a code style issue BTW. You also need to make sure the command-line takes precedence over the config.

I'm actually not sure what you're referring here though because one of the things I did on the original commit here is refactor to try to put all the logic in the same place, since there was some duplication across the file, maybe you can be more specific of what you mean?

@decsny I think the idea is to make sure that west flash --foo always takes precedence over any west config flash.foo setting.

@decsny I think the idea is to make sure that west flash --foo always takes precedence over any west config flash.foo setting.

Yes - and good luck reviewing and maintaining the correct precedence if the code for one were far away from the other.

because one of the things I did on the original commit here is refactor to try to put all the logic in the same place

I noticed that one of your very first versions https://github.com/zephyrproject-rtos/zephyr/blob/cef1c2527e0686f6dcf60174aa192c7fbae98d32/scripts/west_commands/run_common.py still had some "skip" code in two places (lines 250 and 730). You force-pushed and regrouped everything that while I was making that comment #96115 (review). I didn't check again and did not notice the change, apologies!

@decsny I think the idea is to make sure that west flash --foo always takes precedence over any west config flash.foo setting.

Yes - and good luck reviewing and maintaining the correct precedence if the code for one were far away from the other.

because one of the things I did on the original commit here is refactor to try to put all the logic in the same place

I noticed that one of your very first versions https://github.com/zephyrproject-rtos/zephyr/blob/cef1c2527e0686f6dcf60174aa192c7fbae98d32/scripts/west_commands/run_common.py still had some "skip" code in two places (lines 250 and 730). You force-pushed and regrouped everything that while I was making that comment #96115 (review). I didn't check again and did not notice the change, apologies!

ok, I dont remember if that was the case, I didnt even remember i pushed multiple times

anyways, I updated the PR for the multiple command and symmetrical cli as requested, please check if the logic is how you imagined

Would still be nice to have a test for this.

Where do we test the rest of the code in this file?

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud